Can this be moved to base.models.BaseModel.Meta?

You have to explicitly inherit from the class Meta(BaseModel.Meta)

Adding create_project here makes sense for the Project model! but for other models, they could either go on that model or the BaseModel with a dynamic method.

I tried moving permissions to the BaseModel, but it turns out Django creates duplicate permissions when they’re inherited one for the BaseModel itself and another for each subclass like Project.
We could instead handle this through a signal that automatically creates model-level permissions for each relevant model at startup.

I don't mind having all the permissions listed under Project.Meta because I think we will move them to Organization.Meta later. Otherwise, I would say put them each under a relevant model like ProcessingService.Meta

The assign_model_level_permissions method hardcodes the content type to Project, which limits the GlobalRole class to only Project permissions. Consider making this more generic by accepting a model class parameter or deriving the content type from the permission codename.

The method calls super().check_object_level_permission(user, action) on line 329, but check_permission is the entry point that should be called. This bypasses the logic in the parent's check_permission method. Call super().check_permission(user, action) instead to ensure proper permission routing.

The parameter order differs from the parent class method in ami/base/models.py line 326, which has project before user. This inconsistency could lead to errors when calling the method. Align the parameter order with the base class.

Hardcoding app_label to 'main' makes this method inflexible for models in other apps. Use self._meta.app_label instead to automatically determine the correct app label.

I agree with this suggestion, since models in other apps use the BaseModel, the constructed strings will be incorrect. For example:
"create": f"{app_label}.create_{model}"
will be
"create": f"main.create_job" instead of "create": f"jobs.create_job"

The hardcoded app_label = 'main' with a commented-out self._meta.app_label suggests this should use the model's actual app label. Use self._meta.app_label to ensure correct permissions are checked for models outside the 'main' app.

I agree here as well

Similar to previous comments, hardcoding app_label to 'main' limits reusability. Use cls._meta.app_label to derive the app label from the model class.

This helper method is duplicated in both TestProcessingServiceModelLevelPermissions and TestTaxonModelLevelPermissions classes. Consider extracting it to a shared test base class or utility module to reduce code duplication.

This helper method is also duplicated across test classes. Extract it along with _assign_user_permission_and_reset_caches to a shared location to improve maintainability.

Nice catch

How does this know to check the create permission?

Will you add a comment about why get_permission needs to be overwritten here? Is it because Project has no project accessor?

This is helpful, thanks.

Can we rename this to AuthorizedUser? or ApprovedUser

Authenticated implies that they are logged-in. But I think authorized or approved are what we want here.

Also the other roles are singular instead of plural, so I would stick with that pattern.

⚠️ Potential issue | 🔴 Critical

Critical: Default model_name parameter causes incorrect ContentType lookup.

The helper methods default model_name="project", but they're used to assign permissions for other models (ProcessingService, Taxon) without overriding this parameter. This causes ContentType.objects.get(model="project") to be used for all models, associating permissions with the wrong content type.

Impact: Tests may pass, but permissions are associated with the Project model instead of ProcessingService or Taxon models, causing permission checks to fail in production.

Apply this diff to fix the issue by deriving the model name from the permission codename:

 def _assign_user_permission_and_reset_caches(
-    self, user, perm_codename: str, app_label: str = "main", model_name: str = "project"
+    self, user, perm_codename: str, app_label: str = "main", model_name: str | None = None
 ):
+    # Derive model name from permission codename if not explicitly provided
+    # E.g., "create_processingservice" -> "processingservice"
+    if model_name is None:
+        # Remove common action prefixes to get model name
+        for prefix in ["create_", "update_", "delete_", "view_"]:
+            if perm_codename.startswith(prefix):
+                model_name = perm_codename[len(prefix):]
+                break
+        else:
+            # Fallback: use the entire codename as model name
+            model_name = perm_codename
+    
     ct = ContentType.objects.get(app_label=app_label, model=model_name)
     perm, _ = Permission.objects.get_or_create(
         codename=perm_codename,
         content_type=ct,
         defaults={"name": f"Can {perm_codename.replace('_', ' ')}"},
     )

     user.user_permissions.add(perm)

     # Clear cached permissions
     for attr in ["_perm_cache", "_user_perm_cache", "_group_perm_cache"]:
         if hasattr(user, attr):
             delattr(user, attr)
     user.refresh_from_db()

     return perm

 def _remove_user_permission_and_reset_cache(
-    self, user, perm_codename: str, app_label: str = "main", model_name: str = "project"
+    self, user, perm_codename: str, app_label: str = "main", model_name: str | None = None
 ):
+    # Derive model name from permission codename if not explicitly provided
+    if model_name is None:
+        for prefix in ["create_", "update_", "delete_", "view_"]:
+            if perm_codename.startswith(prefix):
+                model_name = perm_codename[len(prefix):]
+                break
+        else:
+            model_name = perm_codename
+    
     try:
         ct = ContentType.objects.get(app_label=app_label, model=model_name)
         perm = Permission.objects.get(codename=perm_codename, content_type=ct)
         user.user_permissions.remove(perm)
     except Permission.DoesNotExist:
         return False

     # Clear cached permissions
     for attr in ["_perm_cache", "_user_perm_cache", "_group_perm_cache"]:
         if hasattr(user, attr):
             delattr(user, attr)
     user.refresh_from_db()

     return True

In ami/main/tests.py around lines 3479-3522, the helper methods default
model_name="project" which causes ContentType lookups to always use the Project
model; change the signature of both helpers to default model_name=None and, if
model_name is None, derive it from the perm_codename by taking the last segment
after splitting on '_' (e.g., perm_codename.split('_')[-1].lower()), then use
that derived model_name for ContentType.objects.get(...); keep the rest of the
logic (creating/getting Permission, adding/removing, clearing caches,
refresh_from_db) unchanged.

@mohamedelabbas1996 will you make it extra clear in the docstrings that the permissions are managed under the project model? If we had an "Organization" model they may go there instead, but currently all permissions are based on a users association to a project. This PR introduces some actions that happen outside of an existing project (e.g. creating a new project), but there is not a another appropriate place to put those. So we are keeping them on the Project class level, a project instance is not required to make these checks.

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!